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/tests/header-and-search.spec.ts b/packages/ui-kit/e2e/tests/header-and-search.spec.ts index 2e7f43c..c8f355a 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, 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'); diff --git a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts index 0f1ea7e..7c89153 100644 --- a/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts +++ b/packages/ui-kit/e2e/tests/mobile-dialog.spec.ts @@ -132,6 +132,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'); diff --git a/packages/ui-kit/e2e/tests/utils.ts b/packages/ui-kit/e2e/tests/utils.ts index 28b6a24..d3e3adb 100644 --- a/packages/ui-kit/e2e/tests/utils.ts +++ b/packages/ui-kit/e2e/tests/utils.ts @@ -11,6 +11,7 @@ export const fixtures = { inlineSelection: '/e2e/fixtures/inline-selection.html', plainMenu: '/e2e/fixtures/plain-menu.html', nestedInput: '/e2e/fixtures/nested-input.html', + mobileNestedInput: '/e2e/fixtures/mobile-nested-input.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 a9f5a03..6111843 100644 --- a/packages/ui-kit/src/popover/popover-desktop.ts +++ b/packages/ui-kit/src/popover/popover-desktop.ts @@ -249,6 +249,7 @@ export class PopoverDesktop extends PopoverAbstract { if (this.search !== undefined) { this.search.updateItems(this.itemsDefault); + this.search.reapplyQuery(); } } @@ -261,6 +262,7 @@ export class PopoverDesktop extends PopoverAbstract { if (this.search !== undefined) { this.search.updateItems(this.itemsDefault); + this.search.reapplyQuery(); } } diff --git a/packages/ui-kit/src/popover/popover-mobile.ts b/packages/ui-kit/src/popover/popover-mobile.ts index 5397d1c..057e1ec 100644 --- a/packages/ui-kit/src/popover/popover-mobile.ts +++ b/packages/ui-kit/src/popover/popover-mobile.ts @@ -61,6 +61,13 @@ 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 + */ + private isLevelFlippable = true; + /** * Construct the instance * @param params - popover params object @@ -120,6 +127,14 @@ export class PopoverMobile extends PopoverAbstract { this.history.push({ items: params.items }); } + /** + * 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; + } + /** * Open popover */ @@ -133,7 +148,10 @@ export class PopoverMobile extends PopoverAbstract { 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 }); @@ -169,6 +187,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; @@ -193,7 +212,7 @@ export class PopoverMobile extends PopoverAbstract { */ protected override showNestedItems(item: PopoverItemDefault): void { /** 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) { @@ -201,7 +220,7 @@ 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); } }; @@ -210,6 +229,7 @@ export class PopoverMobile extends PopoverAbstract { this.history.push({ title: item.title, items: item.children, + isFlippable: item.isChildrenFlippable, }); } @@ -287,7 +307,7 @@ export class PopoverMobile extends PopoverAbstract { * Flipper to point at, so the trap's own first stop is used instead */ private focusFirstElement(): void { - if (this.flippableElements.length > 0) { + if (this.isLevelFlippable && this.flippableElements.length > 0) { this.flipper?.focusFirst(); return; @@ -323,8 +343,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(); @@ -337,7 +358,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(); @@ -358,6 +379,8 @@ export class PopoverMobile extends PopoverAbstract { this.renderItems(this.items); + this.isLevelFlippable = isFlippable; + if (!this.isHidden) { /** * Deactivated before being re-activated, so that the Flipper drops its cursor while it @@ -365,7 +388,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 */