From b64fa2cb6d265aff4c6da2d584509c77a6969383 Mon Sep 17 00:00:00 2001 From: George Berezhnoy Date: Sat, 29 Aug 2026 18:55:09 +0100 Subject: [PATCH] Keep search results and nested-level keys in sync on mobile Two gaps reported while reviewing the a11y work, left out of that branch to keep it reviewable. Search results went stale whenever the item list changed underneath them: addItem()/removeItemByName() updated the searchable set but never re-ran the query, so a newcomer joined the results whether it matched or not and the announced count went with it. SearchInput.reapplyQuery() re-runs whatever is already typed, and PopoverDesktop calls it after either mutation. The mobile popover ignored children.isFlippable. It renders nested levels into the same panel rather than into a popover of their own, so the Flipper navigating the root list carried on claiming the arrows and Enter after drilling in - an item built around a text input could never receive them. The flag now travels with the level through PopoverStatesHistory, and a level that opted out leaves the Flipper deactivated, its items becoming individual stops of the panel's Tab trap instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01F3JEisQDVD82VYAp55Lhcp --- .../e2e/fixtures/mobile-nested-input.html | 70 +++++++++++++++++++ .../e2e/tests/header-and-search.spec.ts | 32 ++++++++- .../ui-kit/e2e/tests/mobile-dialog.spec.ts | 23 ++++++ packages/ui-kit/e2e/tests/utils.ts | 1 + .../components/search-input/search-input.ts | 18 +++++ .../ui-kit/src/popover/popover-desktop.ts | 2 + packages/ui-kit/src/popover/popover-mobile.ts | 45 ++++++++++-- .../popover/utils/popover-states-history.ts | 17 +++++ 8 files changed, 200 insertions(+), 8 deletions(-) create mode 100644 packages/ui-kit/e2e/fixtures/mobile-nested-input.html 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 */