From 3e71c8a81748d86c07258f5f952ef51efce1f8c4 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Fri, 20 Oct 2023 01:09:12 +0000 Subject: [PATCH 1/5] Allow iterateFocusableElements options from focus-zone --- src/focus-zone.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/focus-zone.ts b/src/focus-zone.ts index c4bbcb4..157c42d 100644 --- a/src/focus-zone.ts +++ b/src/focus-zone.ts @@ -1,6 +1,6 @@ import {polyfill as eventListenerSignalPolyfill} from './polyfills/event-listener-signal.js' import {isMacOS} from './utils/user-agent.js' -import {iterateFocusableElements} from './utils/iterate-focusable-elements.js' +import {IterateFocusableElements, iterateFocusableElements} from './utils/iterate-focusable-elements.js' import {uniqueId} from './utils/unique-id.js' eventListenerSignalPolyfill() @@ -114,7 +114,7 @@ const KEY_TO_DIRECTION = { /** * Options that control the behavior of the arrow focus behavior. */ -export interface FocusZoneSettings { +export type FocusZoneSettings = IterateFocusableElements & { /** * Choose the behavior applied in cases where focus is currently at either the first or * last element of the container. @@ -504,8 +504,13 @@ export function focusZone(container: HTMLElement, settings?: FocusZoneSettings): } } + const iterateFocusableElementsOptions: IterateFocusableElements = { + reverse: settings?.reverse, + strict: settings?.strict, + onlyTabbable: settings?.onlyTabbable, + } // Take all tabbable elements within container under management - beginFocusManagement(...iterateFocusableElements(container)) + beginFocusManagement(...iterateFocusableElements(container, iterateFocusableElementsOptions)) // Open the first tabbable element for tabbing const initialElement = @@ -519,14 +524,14 @@ export function focusZone(container: HTMLElement, settings?: FocusZoneSettings): for (const mutation of mutations) { for (const removedNode of mutation.removedNodes) { if (removedNode instanceof HTMLElement) { - endFocusManagement(...iterateFocusableElements(removedNode)) + endFocusManagement(...iterateFocusableElements(removedNode, iterateFocusableElementsOptions)) } } } for (const mutation of mutations) { for (const addedNode of mutation.addedNodes) { if (addedNode instanceof HTMLElement) { - beginFocusManagement(...iterateFocusableElements(addedNode)) + beginFocusManagement(...iterateFocusableElements(addedNode, iterateFocusableElementsOptions)) } } } From d6251ee1a0bbb37db930a43d1e7afca320944326 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Fri, 20 Oct 2023 01:09:52 +0000 Subject: [PATCH 2/5] Remove focus if display none --- src/utils/iterate-focusable-elements.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/utils/iterate-focusable-elements.ts b/src/utils/iterate-focusable-elements.ts index 8dacb5d..3a4a58d 100644 --- a/src/utils/iterate-focusable-elements.ts +++ b/src/utils/iterate-focusable-elements.ts @@ -98,10 +98,12 @@ export function isFocusable(elem: HTMLElement, strict = false): boolean { // Each of the conditions checked below require a reflow, thus are gated by the `strict` // argument. If any are true, the element is not focusable, even if tabindex is set. if (strict) { + const style = getComputedStyle(elem) const sizeInert = elem.offsetWidth === 0 || elem.offsetHeight === 0 - const visibilityInert = ['hidden', 'collapse'].includes(getComputedStyle(elem).visibility) + const visibilityInert = ['hidden', 'collapse'].includes(style.visibility) + const displayInert = style.display === 'none' || !elem.offsetParent const clientRectsInert = elem.getClientRects().length === 0 - if (sizeInert || visibilityInert || clientRectsInert) { + if (sizeInert || visibilityInert || clientRectsInert || displayInert) { return false } } From 82dc29ddae0e1726ff6d14c8588c1af4f7896ce8 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Fri, 20 Oct 2023 01:10:06 +0000 Subject: [PATCH 3/5] Add tests for strict option --- src/__tests__/focus-zone.test.tsx | 80 +++++++++++++++++++ .../iterate-focusable-elements.test.tsx | 67 ++++++++++++++++ 2 files changed, 147 insertions(+) diff --git a/src/__tests__/focus-zone.test.tsx b/src/__tests__/focus-zone.test.tsx index 1ddfc3c..9d94cbb 100644 --- a/src/__tests__/focus-zone.test.tsx +++ b/src/__tests__/focus-zone.test.tsx @@ -21,6 +21,26 @@ beforeAll(() => { getClientRects: { get: () => () => [42], }, + offsetParent: { + get() { + // eslint-disable-next-line @typescript-eslint/no-this-alias + for (let element = this; element; element = element.parentNode) { + if (element.style?.display?.toLowerCase() === 'none') { + return null + } + } + + if (this.style?.position?.toLowerCase() === 'fixed') { + return null + } + + if (this.tagName.toLowerCase() in ['html', 'body']) { + return null + } + + return this.parentNode + }, + }, }) } catch { // ignore @@ -569,3 +589,63 @@ it('Should handle elements being reordered', async () => { controller.abort() }) + +it('Should ignore hidden elements if strict', async () => { + const user = userEvent.setup() + const {container} = render( +
+ + + +
+
+ +
+
+
+
+ +
+
+ +
, + ) + const focusZoneContainer = container.querySelector('#focusZone')! + const allButtons = focusZoneContainer.querySelectorAll('button') + const firstButton = allButtons[0] + const lastButton = allButtons[allButtons.length - 1] + const controller = focusZone(focusZoneContainer, {strict: true}) + + firstButton.focus() + expect(document.activeElement).toEqual(firstButton) + + await user.keyboard('{arrowdown}') + expect(document.activeElement).toEqual(lastButton) + + controller.abort() +}) + +it('Shoud move to tabbable elements if onlyTabbable', async () => { + const user = userEvent.setup() + const {container} = render( +
+ + + +
, + ) + + const focusZoneContainer = container.querySelector('#focusZone')! + const allButtons = focusZoneContainer.querySelectorAll('button') + const firstButton = allButtons[0] + const lastButton = allButtons[allButtons.length - 1] + const controller = focusZone(focusZoneContainer, {onlyTabbable: true}) + + firstButton.focus() + expect(document.activeElement).toEqual(firstButton) + + await user.keyboard('{arrowdown}') + expect(document.activeElement).toEqual(lastButton) + + controller.abort() +}) diff --git a/src/__tests__/iterate-focusable-elements.test.tsx b/src/__tests__/iterate-focusable-elements.test.tsx index 7c309cc..36bad14 100644 --- a/src/__tests__/iterate-focusable-elements.test.tsx +++ b/src/__tests__/iterate-focusable-elements.test.tsx @@ -2,6 +2,46 @@ import React from 'react' import {iterateFocusableElements} from '../utils/iterate-focusable-elements.js' import {render} from '@testing-library/react' +// Since we use strict checks for size and parent, we need to mock these +// properties that Jest does not populate. +beforeAll(() => { + try { + Object.defineProperties(HTMLElement.prototype, { + offsetHeight: { + get: () => 42, + }, + offsetWidth: { + get: () => 42, + }, + getClientRects: { + get: () => () => [42], + }, + offsetParent: { + get() { + // eslint-disable-next-line @typescript-eslint/no-this-alias + for (let element = this; element; element = element.parentNode) { + if (element.style?.display?.toLowerCase() === 'none') { + return null + } + } + + if (this.style?.position?.toLowerCase() === 'fixed') { + return null + } + + if (this.tagName.toLowerCase() in ['html', 'body']) { + return null + } + + return this.parentNode + }, + }, + }) + } catch { + // ignore + } +}) + it('Should iterate through focusable elements only', () => { const {container} = render(
@@ -57,3 +97,30 @@ it('Should iterate through focusable elements in reverse', () => { expect(focusable[3].tagName.toLowerCase()).toEqual('input') expect(focusable[4].tagName.toLowerCase()).toEqual('textarea') }) + +it('Should ignore hidden elements if strict', async () => { + const {container} = render( +
+ + + +
+
+ +
+
+
+
+ +
+
+ +
, + ) + const focusable = Array.from(iterateFocusableElements(container as HTMLElement, {strict: true})) + expect(focusable.length).toEqual(2) + expect(focusable[0].tagName.toLowerCase()).toEqual('button') + expect(focusable[0].textContent).toEqual('Apple') + expect(focusable[1].tagName.toLowerCase()).toEqual('button') + expect(focusable[1].textContent).toEqual('Cantaloupe') +}) From 7a7faa9c604ea23f10edd68ed66c35e790a5dc1a Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Fri, 20 Oct 2023 01:26:00 +0000 Subject: [PATCH 4/5] Add changeset --- .changeset/neat-timers-tease.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/neat-timers-tease.md diff --git a/.changeset/neat-timers-tease.md b/.changeset/neat-timers-tease.md new file mode 100644 index 0000000..ac1cfe0 --- /dev/null +++ b/.changeset/neat-timers-tease.md @@ -0,0 +1,5 @@ +--- +'@primer/behaviors': minor +--- + +Add IterateFocusableElements options to focusZone From 00478dec1659a2e4deb1b47653767d8082963015 Mon Sep 17 00:00:00 2001 From: Stephanie Hong <41085564+JelloBagel@users.noreply.github.com> Date: Fri, 20 Oct 2023 01:41:08 +0000 Subject: [PATCH 5/5] Add offsetParent to focus trap tests --- src/__tests__/focus-trap.test.tsx | 32 +++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/src/__tests__/focus-trap.test.tsx b/src/__tests__/focus-trap.test.tsx index b4e1588..68ed72e 100644 --- a/src/__tests__/focus-trap.test.tsx +++ b/src/__tests__/focus-trap.test.tsx @@ -17,6 +17,26 @@ beforeAll(() => { getClientRects: { get: () => () => [42], }, + offsetParent: { + get() { + // eslint-disable-next-line @typescript-eslint/no-this-alias + for (let element = this; element; element = element.parentNode) { + if (element.style?.display?.toLowerCase() === 'none') { + return null + } + } + + if (this.style?.position?.toLowerCase() === 'fixed') { + return null + } + + if (this.tagName.toLowerCase() in ['html', 'body']) { + return null + } + + return this.parentNode + }, + }, }) } catch { // ignore @@ -40,7 +60,7 @@ it('Should initially focus the first element when activated', () => { const controller = focusTrap(trapContainer) expect(document.activeElement).toEqual(firstButton) - controller.abort() + controller?.abort() }) it('Should initially focus the initialFocus element when specified', () => { @@ -57,7 +77,7 @@ it('Should initially focus the initialFocus element when specified', () => { const controller = focusTrap(trapContainer, secondButton) expect(document.activeElement).toEqual(secondButton) - controller.abort() + controller?.abort() }) it('Should prevent focus from exiting the trap, returns focus to first element', async () => { @@ -86,7 +106,7 @@ it('Should prevent focus from exiting the trap, returns focus to first element', await user.tab() expect(document.activeElement).toEqual(firstButton) - controller.abort() + controller?.abort() lastButton.focus() await user.tab() @@ -125,7 +145,7 @@ it('Should cycle focus from last element to first element and vice-versa', async await user.tab({shift: true}) expect(document.activeElement).toEqual(lastButton) - controller.abort() + controller?.abort() }) it('Should should release the trap when the signal is aborted', async () => { @@ -154,7 +174,7 @@ it('Should should release the trap when the signal is aborted', async () => { await user.tab() expect(document.activeElement).toEqual(firstButton) - controller.abort() + controller?.abort() lastButton.focus() await user.tab() @@ -218,5 +238,5 @@ it('Should handle dynamic content', async () => { await user.tab({shift: true}) expect(document.activeElement).toEqual(secondButton) - controller.abort() + controller?.abort() })