diff --git a/packages/ui-kit/e2e/fixtures/mobile-nested-input.html b/packages/ui-kit/e2e/fixtures/mobile-nested-input.html new file mode 100644 index 0000000..963272a --- /dev/null +++ b/packages/ui-kit/e2e/fixtures/mobile-nested-input.html @@ -0,0 +1,70 @@ + + + + + + UI Kit e2e — mobile with a non-flippable nested level + + +
+ +
+ +
+
+ + + + diff --git a/packages/ui-kit/e2e/fixtures/mobile-self-closing-children.html b/packages/ui-kit/e2e/fixtures/mobile-self-closing-children.html new file mode 100644 index 0000000..935384d --- /dev/null +++ b/packages/ui-kit/e2e/fixtures/mobile-self-closing-children.html @@ -0,0 +1,61 @@ + + + + + UI Kit e2e — mobile with a self-closing nested level + + +
+
+ +
+
+ + + + diff --git a/packages/ui-kit/e2e/tests/header-and-search.spec.ts b/packages/ui-kit/e2e/tests/header-and-search.spec.ts index 2e7f43c..8b9f138 100644 --- a/packages/ui-kit/e2e/tests/header-and-search.spec.ts +++ b/packages/ui-kit/e2e/tests/header-and-search.spec.ts @@ -1,5 +1,5 @@ import { expect, test } from '@playwright/test'; -import { hidePopover, showPopover } from './utils'; +import { addItem, hidePopover, removeItemByName, showPopover } from './utils'; test.describe('search input', () => { test.beforeEach(async ({ page }) => { @@ -94,6 +94,36 @@ test.describe('search input', () => { await expect(page.getByRole('menuitem', { name: 'Simple Item' })).toBeFocused(); }); + test('an item added while a query is typed is filtered by that query', async ({ page }) => { + /** + * Changing the item list leaves the results describing a list that no longer exists: the + * newcomer used to show up among the matches whether it matched or not, and the announced + * count went with it + */ + await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); + await expect(page.getByRole('menuitemradio')).toHaveCount(2); + + await addItem(page, { + title: 'Align Right', + name: 'align-right', + toggle: 'align', + }); + + const matchesAfterAdding = 3; + + await expect(page.getByRole('menuitemradio')).toHaveCount(matchesAfterAdding); + await expect(page.getByRole('status').first()).toHaveText(`${matchesAfterAdding} results`); + + await addItem(page, { + title: 'Strikethrough', + name: 'strike', + }); + + /** Does not match, so it stays out of the results rather than joining them */ + await expect(page.getByRole('menuitem', { name: 'Strikethrough' })).toHaveCount(0); + await expect(page.locator('[data-item-name="strike"]')).toBeHidden(); + }); + test('arrow navigation after clicking a result stays within the matches', async ({ page }) => { await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); @@ -114,6 +144,44 @@ test.describe('search input', () => { await page.keyboard.press('ArrowDown'); await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); }); + + test('keeps the navigation cursor on the focused result when an item is added', async ({ page }) => { + await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); + + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('menuitemradio', { name: 'Align Left' })).toBeFocused(); + + /** + * Adding an item reapplies the query, which restarts the Flipper. Without a cursor to + * resume from, the next arrow press would silently start over from the top of the list + * while the focus stayed where the user left it + */ + await addItem(page, { + title: 'Added item', + name: 'added', + }); + + await expect(page.getByRole('menuitemradio', { name: 'Align Left' })).toBeFocused(); + + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); + }); + + test('keeps the navigation cursor on the focused result when an item is removed', async ({ page }) => { + await page.getByRole('searchbox', { name: 'Search' }).fill('Align'); + + await page.keyboard.press('ArrowDown'); + await page.keyboard.press('ArrowDown'); + await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); + + /** Filtered out by the query, so the results the user is navigating do not change */ + await removeItemByName(page, 'bold'); + + await expect(page.getByRole('menuitemradio', { name: 'Align Center' })).toBeFocused(); + + await page.keyboard.press('ArrowUp'); + await expect(page.getByRole('menuitemradio', { name: 'Align Left' })).toBeFocused(); + }); }); test.describe('mobile popover header', () => { diff --git a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts index 4f164fc..3959101 100644 --- a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts +++ b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts @@ -135,6 +135,29 @@ test.describe('mobile popover', () => { await expect(back).toBeFocused(); }); + test('a nested level with isFlippable false leaves its keys to its own controls', async ({ page }) => { + /** + * Nested levels render into the same panel as the root one, so the Flipper that navigates + * the root list would carry on claiming the arrows and Enter here too - and an item built + * around a text input needs both for itself + */ + await showPopover(page, 'mobileNestedInput'); + + await page.keyboard.press('ArrowDown'); + await page.keyboard.press('Enter'); + + const input = page.getByRole('textbox', { name: 'Nested input' }); + + await expect(input).toBeVisible(); + + await input.fill('editorjs'); + await page.keyboard.press('ArrowDown'); + + /** The Flipper would have moved the focus off to the next item by now */ + await expect(input).toBeFocused(); + await expect(input).toHaveValue('editorjs'); + }); + test('Enter drills into a nested item', async ({ page }) => { await showPopover(page, 'mobile'); @@ -311,6 +334,94 @@ test.describe('mobile dialog focus edge cases', () => { }); }); +test.describe('nested levels', () => { + test('returns to the root level when a nested one closes itself as it opens', async ({ page }) => { + await showPopover(page, 'mobileSelfClosingChildren'); + + await page.getByText('Closes itself').click(); + + /** + * The level pushes its state before rendering, so the close that arrives while it is still + * opening pops that state rather than the root one underneath it + */ + await expect(page.getByRole('menuitem', { name: 'Simple item' })).toBeVisible(); + await expect(page.getByRole('menuitem', { name: 'Closes itself' })).toBeVisible(); + await expect(page.getByRole('menuitem', { name: 'Never seen' })).toHaveCount(0); + }); +}); + +test.describe('non-flippable nested level', () => { + test.beforeEach(async ({ page }) => { + await showPopover(page, 'mobileNestedInput'); + + await page.getByText('Has children').click(); + }); + + test('leaves the arrows to the items, however the focus got there', async ({ page }) => { + const input = page.getByLabel('Nested input'); + + await input.focus(); + + /** + * Focus arriving on an item of a level that opted out must not move the navigation cursor + * there: that would re-activate the Flipper, and the arrows would stop reaching the input + */ + await page.keyboard.press('ArrowDown'); + await expect(input).toBeFocused(); + + await page.keyboard.press('ArrowUp'); + await expect(input).toBeFocused(); + }); + + test('walks its items with Tab instead', async ({ page }) => { + await page.keyboard.press('Tab'); + await expect(page.getByLabel('Nested input')).toBeFocused(); + + await page.keyboard.press('Tab'); + await expect(page.getByRole('menuitem', { name: 'Child A' })).toBeFocused(); + }); +}); + +test.describe('reopening', () => { + test('comes back to the root level after being closed on a nested one', async ({ page }) => { + await showPopover(page, 'mobileNestedInput'); + + await page.getByText('Has children').click(); + + await expect(page.getByRole('menuitem', { name: 'Child A' })).toBeVisible(); + + await page.keyboard.press('Escape'); + await callShow(page); + + /** + * Closing resets the level history, so the panel has to come back up showing the root items + * rather than the level the user happened to be on when it was dismissed + */ + await expect(page.getByRole('menuitem', { name: 'Simple item' })).toBeVisible(); + await expect(page.getByRole('menuitem', { name: 'Child A' })).toHaveCount(0); + }); + + test('does not restore keyboard navigation to a level that opted out of it', async ({ page }) => { + await showPopover(page, 'mobileNestedInput'); + + await page.getByText('Has children').click(); + await page.keyboard.press('Escape'); + await callShow(page); + + /** + * The nested level is not navigable, and reopening used to leave its items on screen while + * restoring the root's own flippability - the arrows would then navigate a level whose + * items need those keys for themselves + */ + await expect(page.getByRole('menuitem', { name: 'Simple item' })).toBeFocused(); + + await page.keyboard.press('ArrowDown'); + + await expect(page.getByRole('menuitem', { name: 'Has children' })).toBeFocused(); + await expect(page.getByLabel('Nested input')).toHaveCount(0); + }); +}); + test.describe('mobile dialog name', () => { test('is named even without a label of its own', async ({ page }) => { await showPopover(page, 'mobile'); diff --git a/packages/ui-kit/e2e/tests/utils.ts b/packages/ui-kit/e2e/tests/utils.ts index 0794b10..3e37168 100644 --- a/packages/ui-kit/e2e/tests/utils.ts +++ b/packages/ui-kit/e2e/tests/utils.ts @@ -16,6 +16,8 @@ export const fixtures = { confirmationToggle: '/e2e/fixtures/confirmation-toggle.html', mobilePlain: '/e2e/fixtures/mobile-plain.html', mobileEmpty: '/e2e/fixtures/mobile-empty.html', + mobileNestedInput: '/e2e/fixtures/mobile-nested-input.html', + mobileSelfClosingChildren: '/e2e/fixtures/mobile-self-closing-children.html', } as const; /** diff --git a/packages/ui-kit/src/popover/components/search-input/search-input.ts b/packages/ui-kit/src/popover/components/search-input/search-input.ts index 730847b..ae50eb9 100644 --- a/packages/ui-kit/src/popover/components/search-input/search-input.ts +++ b/packages/ui-kit/src/popover/components/search-input/search-input.ts @@ -106,6 +106,24 @@ export class SearchInput extends EventsDispatcher { this.items = items; } + /** + * Runs the query that is already typed in against the item list again. + * + * Adding or removing an item leaves the results describing a list that no longer exists: + * the newcomer shows up among the matches whether it matches or not, and the reported count + * is off. Re-running the query brings both back in sync without the user retyping it + */ + public reapplyQuery(): void { + if (this.searchQuery === undefined || this.searchQuery === '') { + return; + } + + this.emit(SearchInputEvent.Search, { + query: this.searchQuery, + items: this.foundItems, + }); + } + /** * Returns search field element */ diff --git a/packages/ui-kit/src/popover/popover-desktop.ts b/packages/ui-kit/src/popover/popover-desktop.ts index 2519584..93c60dd 100644 --- a/packages/ui-kit/src/popover/popover-desktop.ts +++ b/packages/ui-kit/src/popover/popover-desktop.ts @@ -259,6 +259,7 @@ export class PopoverDesktop extends PopoverAbstract { if (this.search !== undefined) { this.search.updateItems(this.itemsDefault); + this.search.reapplyQuery(); } } @@ -271,6 +272,7 @@ export class PopoverDesktop extends PopoverAbstract { if (this.search !== undefined) { this.search.updateItems(this.itemsDefault); + this.search.reapplyQuery(); } } @@ -876,6 +878,15 @@ export class PopoverDesktop extends PopoverAbstract { const flippableElements = data.query === '' ? this.flippableElements : data.items.map(item => (item as PopoverItem).getElement()); if (this.flipper?.isActivated === true) { + /** + * Noted before the Flipper is restarted: re-filtering can happen while an item rather than + * the search field holds the focus (adding or removing an item reapplies the query), and + * restarting the Flipper without a cursor leaves the highlight and the roving tabindex + * behind on the first item while the real focus stays where the user left it + */ + const active = document.activeElement; + const focusedItem = active instanceof HTMLElement && this.nodes.items.contains(active) ? active : null; + /** Update flipper items with only visible */ this.flipper.deactivate(); this.flipper.activate(flippableElements as HTMLElement[]); @@ -886,6 +897,14 @@ export class PopoverDesktop extends PopoverAbstract { * scoped to the currently visible items, so Tab still reaches the filtered results. */ this.toggleItemsTabbable(true, flippableElements as HTMLElement[]); + + /** + * Only when an item was focused: typing in the search field must not move the cursor onto + * an item, since that is what would take the focus away from the query being typed + */ + if (focusedItem !== null) { + this.moveCursorTo(focusedItem); + } } }; diff --git a/packages/ui-kit/src/popover/popover-mobile.ts b/packages/ui-kit/src/popover/popover-mobile.ts index 53f193d..1aac427 100644 --- a/packages/ui-kit/src/popover/popover-mobile.ts +++ b/packages/ui-kit/src/popover/popover-mobile.ts @@ -57,6 +57,20 @@ export class PopoverMobile extends PopoverAbstract { */ private previouslyFocusedElement: HTMLElement | null = null; + /** + * Whether the items currently on screen take part in keyboard navigation. + * Nested levels opt out of it via children.isFlippable, and since they are rendered into the + * same panel rather than into a popover of their own, the flag has to travel with the level. + * The root level's own value comes from the 'flippable' construction param + */ + private isLevelFlippable: boolean; + + /** + * Whether the items on screen belong to a nested level rather than to the root one. + * The panel renders every level into the same container, so this is what tells the two apart + */ + private isNestedLevelRendered = false; + /** * Construct the instance * @param params - popover params object @@ -106,7 +120,9 @@ export class PopoverMobile extends PopoverAbstract { * A popover built with 'flippable: false' opts out of keyboard navigation altogether, and * then every item becomes an individual stop of the trap instead - see toggleItemsTabbable() */ - if (params.flippable !== false) { + this.isLevelFlippable = params.flippable !== false; + + if (this.isLevelFlippable) { this.flipper = new Flipper({ items: this.flippableElements, focusedItemClass: popoverItemCls.focused, @@ -120,7 +136,29 @@ export class PopoverMobile extends PopoverAbstract { } /* Save state to history for proper navigation between nested and parent popovers */ - this.history.push({ items: params.items }); + this.history.push({ + items: params.items, + isFlippable: this.isLevelFlippable, + }); + } + + /** + * The items share a single Tab stop only while the level they belong to is navigable. + * A level that opted out is walked by Tab like a plain list instead + */ + protected override get hasRovingTabindex(): boolean { + return this.isLevelFlippable && super.hasRovingTabindex; + } + + /** + * A level that opted out of keyboard navigation has no cursor to keep in sync: moving one + * there would re-activate the Flipper and take the arrows away from the items that need them + * for themselves. The Flipper is left holding the previous level's items, which is what + * currently keeps the cursor from landing anywhere - this states the rule rather than + * relying on that + */ + protected override get isFlipperCursorSyncEnabled(): boolean { + return this.isLevelFlippable && super.isFlipperCursorSyncEnabled; } /** @@ -139,11 +177,27 @@ export class PopoverMobile extends PopoverAbstract { this.previouslyFocusedElement = document.activeElement instanceof HTMLElement ? document.activeElement : null; } + /** + * Closing resets the history to the root level, but leaves on screen whatever level the + * user was on. Rendered here rather than on the way out, so that nothing changes under the + * closing animation, and while the dialog still counts as hidden, so this only rebuilds the + * items: the keyboard navigation is set up by the rest of show() below. + * + * Without it a nested level would come back up with the root's own flippability, and one + * that opted out of keyboard navigation would have it turned back on by the activation below + */ + if (this.isNestedLevelRendered) { + this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle, this.history.currentIsFlippable); + } + super.show(); this.scrollLocker.lock(); - this.flipper?.activate(this.flippableElements); + if (this.isLevelFlippable) { + this.flipper?.activate(this.flippableElements); + } + this.toggleItemsTabbable(true); this.listeners.on(document, 'keydown', this.handleKeyDown as (event: Event) => void, { capture: true }); @@ -179,6 +233,7 @@ export class PopoverMobile extends PopoverAbstract { this.listeners.off(document, 'keydown', this.handleKeyDown as (event: Event) => void, { capture: true }); this.history.reset(); + this.isLevelFlippable = this.history.currentIsFlippable; this.isHidden = true; @@ -208,8 +263,19 @@ export class PopoverMobile extends PopoverAbstract { * @param item – item object to show nested popover for */ protected override showNestedItems(item: PopoverItemDefault): void { + /** + * Pushed before the level is rendered, and before onChildrenOpen() below is handed a way to + * close it: a close arriving synchronously from there would otherwise pop the root state, + * the only one on the stack at that point, and leave the popover with no state at all + */ + this.history.push({ + title: item.title, + items: item.children, + isFlippable: item.isChildrenFlippable, + }); + /** Show nested items */ - this.updateItemsAndHeader(item.children, item.title); + this.updateItemsAndHeader(item.children, item.title, item.isChildrenFlippable); const close = (parent?: boolean): void => { if (parent === true) { @@ -217,16 +283,11 @@ export class PopoverMobile extends PopoverAbstract { } else { this.history.pop(); - this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle); + this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle, this.history.currentIsFlippable); } }; item.onChildrenOpen(close); - - this.history.push({ - title: item.title, - items: item.children, - }); } /** @@ -303,12 +364,13 @@ export class PopoverMobile extends PopoverAbstract { * Moves focus inside the dialog once it is opened. * * The menu is entered at its first item, so that the arrows navigate from there right away. - * Without a Flipper ('flippable: false'), or with a list that has nothing for it to point at - * (only separators, for example), the trap's own first stop is used instead. And when there - * is no stop at all, the dialog itself takes the focus + * Without a Flipper ('flippable: false'), on a nested level that opted out of keyboard + * navigation, or with a list that has nothing for it to point at (only separators, for + * example), the trap's own first stop is used instead. And when there is no stop at all, + * the dialog itself takes the focus */ private focusFirstElement(): void { - if (this.flipper !== undefined && this.flippableElements.length > 0) { + if (this.flipper !== undefined && this.isLevelFlippable && this.flippableElements.length > 0) { this.flipper.focusFirst(); return; @@ -344,8 +406,9 @@ export class PopoverMobile extends PopoverAbstract { * Removes rendered popover items and header and displays new ones * @param items - new popover items * @param title - new popover header text + * @param isFlippable - false if the new items opted out of keyboard navigation */ - private updateItemsAndHeader(items: PopoverItemParams[], title?: string): void { + private updateItemsAndHeader(items: PopoverItemParams[], title?: string, isFlippable = true): void { /** Re-render header */ if (this.header !== null && this.header !== undefined) { this.header.destroy(); @@ -358,7 +421,7 @@ export class PopoverMobile extends PopoverAbstract { onBackButtonClick: () => { this.history.pop(); - this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle); + this.updateItemsAndHeader(this.history.currentItems, this.history.currentTitle, this.history.currentIsFlippable); }, }); const headerEl = this.header.getElement(); @@ -379,6 +442,11 @@ export class PopoverMobile extends PopoverAbstract { this.renderItems(this.items); + this.isLevelFlippable = isFlippable; + + /** Only a nested level carries a title, the root one is titleless by construction */ + this.isNestedLevelRendered = title !== undefined; + if (!this.isHidden) { /** * Deactivated before being re-activated, so that the Flipper drops its cursor while it @@ -386,7 +454,15 @@ export class PopoverMobile extends PopoverAbstract { * the cursor is left at */ this.flipper?.deactivate(); - this.flipper?.activate(this.flippableElements); + + /** + * A level that opted out of keyboard navigation leaves the Flipper deactivated, so it + * stops claiming the arrows and Enter: an item holding a text input needs those for + * itself. Its items become individual stops of the panel's Tab trap instead + */ + if (this.isLevelFlippable) { + this.flipper?.activate(this.flippableElements); + } /** Element that was focused has just been removed, so focus is moved into the new list */ this.toggleItemsTabbable(true); diff --git a/packages/ui-kit/src/popover/utils/popover-states-history.ts b/packages/ui-kit/src/popover/utils/popover-states-history.ts index 5822c06..1286754 100644 --- a/packages/ui-kit/src/popover/utils/popover-states-history.ts +++ b/packages/ui-kit/src/popover/utils/popover-states-history.ts @@ -13,6 +13,12 @@ interface PopoverStatesHistoryItem { * Popover items */ items: PopoverItemParams[]; + + /** + * False if the items of this state opted out of keyboard navigation. + * Undefined is treated as navigable, which is the default for every level + */ + isFlippable?: boolean; } /** @@ -61,6 +67,17 @@ export class PopoverStatesHistory { return this.history[this.history.length - 1].items; } + /** + * Whether the items of the current state take part in keyboard navigation + */ + public get currentIsFlippable(): boolean { + if (this.history.length === 0) { + return true; + } + + return this.history[this.history.length - 1].isFlippable !== false; + } + /** * Returns history to initial popover state */